Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Границы обращения к метаданным #1837

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

Golovanoff
Copy link
Contributor

@Golovanoff Golovanoff commented Sep 23, 2021

Описание

Диагностика позволяет найти обращение к метаданным (через поиск регулярным выражением) за пределами указанных границ. Например, вне какого-то определенного общего модуля.

Связанные задачи

Closes

Чеклист

Общие

  • Ветка PR обновлена из develop
  • Отладочные, закомментированные и прочие, не имеющие смысла участки кода удалены
  • Изменения покрыты тестами
  • Обязательные действия перед коммитом выполнены (запускал команду gradlew precommit)

Для диагностик

  • Описание диагностики заполнено для обоих языков (присутствуют файлы для обоих языков, для русского заполнено все подробно, перевод на английский можно опустить)

Дополнительно


for (Map.Entry<String, String> entry: metadataBordersParameters.entrySet()) {

if (entry.getKey().isBlank() || entry.getValue().isBlank()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

это условие нужно проверять на этапе configure

и создавать мапу без неверных значений

Copy link
Contributor

@artbear artbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть замечания

@Golovanoff
Copy link
Contributor Author

поправил замечания

Copy link
Contributor

@artbear artbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

и еще немного предложений по оптимизации

@Golovanoff
Copy link
Contributor Author

заработало через visitFile

Copy link
Contributor

@artbear artbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Есть вопросы

docs/diagnostics/MetadataBorders.md Outdated Show resolved Hide resolved
docs/diagnostics/MetadataBorders.md Outdated Show resolved Hide resolved
docs/diagnostics/MetadataBorders.md Outdated Show resolved Hide resolved

| Имя | Тип | Описание | Значение<br>по умолчанию |
|:---------------------------:|:--------:|:------------------------------------------------------------------------------------------------------------:|:------------------------------:|
| `metadataBordersParameters` | `Строка` | `JSON-структура для пар "регулярное выражение для операторов":"регулярное выражение для имени файла модуля"` | `` |
Copy link
Member

@nixel2007 nixel2007 Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Где-то в ранних версиях здесь был пример заполнения. Кажется, стоит его вернуть. Плюс добавить {}

private List<Pattern> statementPatterns = Collections.emptyList();

private static Map<Pattern, Pattern> MapFromJSON(String userSettings) {
ObjectMapper mapper = new ObjectMapper();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

создание ObjectMapper на каждый configure - довольно дорогая операция. лучше его закэшировать или просто указать его конструкторе - его туда подложит спринг.


private static Map<Pattern, Pattern> MapFromJSON(String userSettings) {
ObjectMapper mapper = new ObjectMapper();
MapType mapType = mapper.getTypeFactory().constructMapType(HashMap.class, String.class, String.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapType тоже можно закэшировать в конструторе по принятому инстансу обджект-маппера

Map<String, String> stringMap = mapper.readValue(userSettings, mapType);
return stringMap.entrySet().stream()
.filter(entry -> ! entry.getKey().isBlank() && ! entry.getValue().isBlank())
.collect(Collectors.toMap(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

пробовал делать включать замер производительности? диагностика не вырывается в топ?

}
});

return super.visitStatement(ctx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

если ты проверяешь текст statement целиком, то нет смысла дергать super, т.к. вложенные стэйтменты проверят тот же самый кусок текста. т.е. здесь будет фонить на вложенных конструкциях вида

Если Истина Тогда
  Если Истина Тогда
    ЗапрещенныйВызов(); // тут кинет два срабатывания на одно и то же место
  КонецЕсли;
КонецЕсли;

@@ -0,0 +1,3 @@
diagnosticMessage=Обращение к метаданным за пределами разрешенных границ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

может быть в сообщение стоит добавить регулярку/пару регулярок, на которой произошло срабатывание?

| `metadataBordersParameters` | `Строка` | `JSON-структура для пар "регулярное выражение для операторов":"регулярное выражение для имени файла модуля"` | `` |
| Имя | Тип | Описание | Значение<br>по умолчанию |
|:---------------------------:|:--------:|:---------------------------------------------------------------------------------------:|:------------------------------:|
| `metadataBordersParameters` | `Строка` | `{"регулярное выражение для операторов":"регулярное выражение для имени файла модуля"}` | `` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

стало еще менее понятно :(

Предлагаю что-то вроде:

Строка в виде объекта JSON (в фигурных скобках) с перечислением пар "регулярное выражение для операторов": "регулярное выражение для имени файла модуля". Например, `{"Регистры?Сведений.КонтактнаяИнформация": "CommonModules/РаботаСКонтактами"}`.

Пускай пример будет прям в описании параметра, упростит понимание.

@Override
public ParseTree visitFile(BSLParser.FileContext ctx) {
statementPatterns = metadataBordersParameters.entrySet().stream()
.filter(entry -> ! entry.getValue().matcher(documentContext.getUri().getPath()).find())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а почему пути к файлам используются? почему не mdoRef?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ну и стоит проверять заполненность statementPatterns: для пустой делать возврат и не греть воздух

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а почему пути к файлам используются? почему не mdoRef?

вообще у нее scope захватывает OS, а для оскрипта mdoRef сейчас выглядит в виде uri. с другой стороны, вряд ли ее кто-то будет включать на os

Copy link
Contributor Author

@Golovanoff Golovanoff Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

таки да. по-хорошему, если делать универсальненько и для людей, то надо вообще переделать, как Никита предлагал в телеге - на вхождение в подсистему.
но я такого пока не умею, это ж надо еще и тесты менять - конфу запихивать, разбираться с этим.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ну и стоит проверять заполненность statementPatterns: для пустой делать возврат и не греть воздух

это сделано при формировании мапы паттернов. из стринговой мапы в мапу щаблонов попадают только те элементы, в которых и ключ, и значение не пустые


statementPatterns.forEach(pattern -> {
Matcher matcher = pattern.matcher(ctx.getText());
while (matcher.find()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

вообще не понял - а цикл зачем?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

потому что матчер возвращает срабатывание по одному за раз

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а зачем одну и туже диагностику в цикле добавлять?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100 раз "addDiagnostic(ctx)"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

так, стоп, тут же где-то был другой код, когда в addDiagnostic передается start() и end() от матчера... или это в соседней диагностике?)

@theshadowco
Copy link
Member

@Golovanoff @nixel2007
реквест висит давно. что делать с ним? переначать ?

@Golovanoff
Copy link
Contributor Author

был артуррилион замечаний, я поправил, пришел Никита и спросил "а нафига так-то? ". на этом моменте я сломался 😀
диагностика у меня в конторе работает, пользу приносит, а как ее переделать, чтобы ее приняли в мейн - я хз 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants